-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[14기 팝콘] Step2 상태 관리로 메뉴 관리하기 #284
base: yeomgahui
Are you sure you want to change the base?
Conversation
STEP 1 구현을 마쳤습니다. 개인적으로 구현하면서 아쉬웠던 부분을 생각해보면, menu 추가시 수정 삭제를 위해 addEventListener를 등록 하는데, 메뉴 하나를 추가할때마다 모든 추가, 삭제 버튼의 eventListener를 다시 등록하도록 구현했습니다. => 개인적으로는 하나 추가할때마다 해당 수정, 삭제 버튼의 eventLIstener만을 추가하도록 하고 싶었는데 좋은 방법이 떠오르지 않았습니다. JavaScript 파일을 기능 마다 분리하여서 작성하고 싶었는데, 어떻게 나눌지 모르곘어서 index.js 파일 내에 모든 로직을 다 작성했습니다. 이부분은 차차 다시 생각해보고 수정해보고 싶습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 팝콘님! Step2 구현하느라 고생하셨어요!
리뷰 좀 남겨 보았어요~!
제 생각엔 중복되는 부분을 리팩토링 해보면 좋을 것 같아요! 😀
|
||
const submitMenu = (event) => { | ||
event.preventDefault(); | ||
const keyword = $('#espresso-menu-name').value.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$('#espresso-menu-name')
선언이 중복되고 있는데 윗부분에 한번만 선언해도 되지 않을까요~? 😀
|
||
const $menuName = $targetList.querySelector(".menu-name"); | ||
let updatedMenu = prompt('메뉴명을 수정하세요.', $menuName.innerText) ?? ''; | ||
updatedMenu = updatedMenu.trim().length > 0 ? updatedMenu : $menuName.innerText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updatedMenu = updatedMenu.trim().length > 0 ? updatedMenu : $menuName.innerText; | |
if(updatedMenu.trim().length === 0){ | |
return; | |
} |
이렇게 변경해도 같은 동작을 하지 않을까요~? 지금은 변경되지 않아도 아래 로직이 동작하면서 안해도 되는 처리까지 진행되고 있는 것 같아용!😮
let menuItem = JSON.parse(localStorage.getItem("menu")); | ||
if(!menuItem) { | ||
menuItem = defaultMenuCategory | ||
localStorage.menu = JSON.stringify(menuItem); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let menuItem = JSON.parse(localStorage.getItem("menu")); | |
if(!menuItem) { | |
menuItem = defaultMenuCategory | |
localStorage.menu = JSON.stringify(menuItem); | |
}; | |
let menuItem = JSON.parse(localStorage.getItem("menu")) || defaultMenuCategory; | |
localStorage.menu = JSON.stringify(menuItem); |
이렇게 변경해도 같은 동작을 할 것 같아요!
let savedMenus = JSON.parse(localStorage.getItem('menu')); | ||
savedMenus[category].push({name : menu}); | ||
localStorage.menu = JSON.stringify(savedMenus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON.parse(localStorage.getItem('menu')
와 JSON.stringify
가 중복해서 여러번 호출되고 있는데 함수화 하면 어떨까요~? 😀
const menu = fetchMenu(); | ||
const targetItem = menu[currentCategory][index]; | ||
|
||
let soldOut = !targetItem.hasOwnProperty('soldOut') ? true : targetItem.soldOut ? false : true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로 3항연산자
를 2중으로 쓰게 되면 코드 읽기가 어려워지는 것 같아요 ㅠ
🎯 step2 요구사항 - 상태 관리로 메뉴 관리하기
sold-out
class를 추가하여 상태를 변경한다.Step 2 구현 완료하였습니다.
아쉬운 부분이 많습니다. 가장 먼저 코드가 너무 뒤죽박죽이네요.. 차차 수정하도록 하겠습니다..